Refactor game state by wjt · Pull Request #2235 · endlessm/threadbare

wjt · 2026-05-21T13:52:16Z

GameState mixes the actual load/save logic with the state itself, and
also is based on a mixture of directly accessing the contents of a
ConfigFile object and fields of the GameState node.

State API

Factor all the state itself out to a (shallow) tree of Resource subclasses:

SavedGame
- GlobalState
  - PlayerState
- QuestState (nullable)
  - Quest-specific PlayerState instance
- PerSceneState

Expose the GlobalState, QuestState, and PerSceneState resources on
GameState with short names to save on typing when referring to them. Add
a special GameState.player property which is a proxy for the
quest-specific PlayerState when on a quest and the global PlayerState
otherwise. Update all code in the game to refer to these.

Remove @tool from GameState, and instead short-circuit in other
scripts which need to be @tool but refer to GameState.

Storage

Replace the ConfigFile-based load and save logic with ResourceLoader and
ResourceSaver. This avoids writing bespoke serialisation code & makes it
relatively easy to add new saved fields – drop an @export into the
appropriate state class – at the cost of coupling the savegame format to
these Resource subclasses existing at these exact paths. This is the
approach recommended, among others, by GDQuest.

https://www.gdquest.com/library/save_game_godot4/

The state conceptually needs to refer to Quest resources. Storing these
as resources would mean that the saved_game.tres would have an
ext_resource header for each quest.tres being referred to, and moving
a quest elsewhere in the source tree (or deleting it) will cause the
whole saved_game.tres to fail to load. Mark the Quest and Array[Quest]
as not to be stored, and define stored proxy properties that convert to
and from resource_path strings. This matches how these were previously
stored.

Similarly, the inventory is in terms of InventoryItem resources. Unlike
Quests we don't store these in the game repo but define them inline
wherever they are used. I think this is a bad design - it would be
better to use just the ItemType enum throughout given that we hardcode
exactly 3 items and 2 textures for each - but to avoid this PR getting
even further out of hand, translate these to and from the ItemType enum
member name when storing, as before.

The result of jumping through these hoops is that the saved_game.tres
only refers to the scripts that define aspects of the game state. I
think that's acceptable coupling. I am disappointed to have to write
quite a bit of what amounts to custom serialisation code - I was hoping
to avoid that by using ResourceLoader/ResourceSaver - but it's still a
net improvement I think.

I did not make any attempt to migrate the old save game format to the
new one. This game is small, the state has little impact, and this will
also mean more people see the tutorial. :)

Saving

Only save the state in a few specific places, rather than whenever parts
of the state change (if we remembered to add a _save() call):

When a checkpoint is activated (so that if you quit the game with
Alt+F4 after activating a checkpoint, it is kept);
When reloading the current scene, typically due to being defeated, to
record the loss of life;
When switching to a new scene;
When you use the debug menu to tweak the completed quests.

Resolves #2016

github-actions · 2026-05-21T13:56:33Z

Play this branch at https://play.threadbare.game/branches/endlessm/wjt/refactor-game-state/.

(This launches the game from the start, not directly at the change(s) in this pull request.)

wjt · 2026-05-26T16:50:28Z

 	interact_area.interaction_ended.connect(self._on_interaction_ended)

-	if GameState.incorporating_threads:
+	if GameState.state.quest_state.incorporating_threads:


It's clear that this is less clear than the previous one because you have to use .state.[kind_state] everywhere. I could make it simpler by proxying the quest_state and player_state properties out one layer so you could write: GameState.quest_state.incorporating_threads

or even:

GameState.quest.incorporating_threads

?

Yes I like it! GameState.quest reads as "the state of the quest", 👍 .

The weird thing about this is that QuestState has a quest property so one ends up writing GameState.quest.quest to access it. Tolerable I think.

wjt · 2026-05-26T16:51:37Z

+	if ResourceLoader.exists(SAVE_PATH):
+		state = ResourceLoader.load(SAVE_PATH, "", ResourceLoader.CACHE_MODE_IGNORE)
+
+	if state:
+		state = state.duplicate_deep(Resource.DEEP_DUPLICATE_INTERNAL)


TODO: per #2250 this should be deferred, we should only load the saved state once we know for sure we want to do that rather than use transient state.

This is surprisingly hard to do and this PR has already got quite big so I'll not work on this further for now.

manuq

This is a huge refactor and you make it look so easy (I know it wasn't). Doing the storage with just resources and ResourceLoader/ResourceSaver rather than ConfigFile for INI files is great.

I like how the player state can be one or another, the global or the quest-specific one.

manuq · 2026-05-27T17:59:53Z

 	if next_scene:
-		GameState.set_challenge_start_scene(next_scene)
+		assert(GameState.quest)


This could be written as a conditional, like in other places:

if next_scene and GameState.quest: ...

I asserted here because I think this situation should not occur - in the current model these items should only be found during quests - but maybe it's a bit extreme. I replaced this with:

if next_scene: - assert(GameState.quest) - GameState.quest.challenge_start_scene = next_scene + if GameState.quest: + GameState.quest.challenge_start_scene = next_scene + else: + push_warning("Collectible collected while not on a quest") switch()

manuq · 2026-05-27T18:30:42Z

+	set(new_value):
+		push_error("Do not set GameState.global")


Interesting way of using the setter to forbid setting the property!

It feels like a bit of a kludge but since there is no such thing at the GDScript level as a readonly property, this is the best I could think of!

manuq · 2026-05-27T18:32:47Z

+## inventory. Note that this does not actually switch to the first scene of [param
+## new_quest].
+func start_quest(new_quest: Quest) -> void:
+	# TODO: this suggests that the inventory should be part of QuestState


Yes, although this may change in the near future...

Indeed, that's why I didn't think too hard about it :)

manuq · 2026-05-27T18:34:45Z

-	if err != OK:
-		push_error("Failed to save settings to %s: %s" % [GAME_STATE_PATH, err])
+
+	print("Saving game")


I understand this is not a leftover, but an actual print, now that the game is saved less frequently. So 👍

Actually I left it in by mistake. I'm on the fence about it!

manuq · 2026-05-27T18:45:26Z

+# SPDX-FileCopyrightText: The Threadbare Authors
+# SPDX-License-Identifier: MPL-2.0
+class_name PerSceneState


I see that PerSceneState is not a @tool. In the pull request description you mention:

Remove @tool from GameState, and instead short-circuit in other
scripts which need to be @tool but refer to GameState.

So yes, this one doesn't need to be a tool script.

GameState mixes the actual load/save logic with the state itself, and also is based on a mixture of directly accessing the contents of a ConfigFile object and fields of the GameState node. State API --------- Factor all the state itself out to a (shallow) tree of Resource subclasses: - SavedGame - GlobalState - PlayerState - QuestState (nullable) - Quest-specific PlayerState instance - PerSceneState Expose the GlobalState, QuestState, and PerSceneState resources on GameState with short names to save on typing when referring to them. Add a special GameState.player property which is a proxy for the quest-specific PlayerState when on a quest and the global PlayerState otherwise. Update all code in the game to refer to these. Remove `@tool` from GameState, and instead short-circuit in other scripts which need to be `@tool` but refer to GameState. Storage ------- Replace the ConfigFile-based load and save logic with ResourceLoader and ResourceSaver. This avoids writing bespoke serialisation code & makes it relatively easy to add new saved fields – drop an `@export` into the appropriate state class – at the cost of coupling the savegame format to these Resource subclasses existing at these exact paths. This is the approach recommended, among others, by GDQuest. https://www.gdquest.com/library/save_game_godot4/ The state conceptually needs to refer to Quest resources. Storing these as resources would mean that the `saved_game.tres` would have an `ext_resource` header for each quest.tres being referred to, and moving a quest elsewhere in the source tree (or deleting it) will cause the whole `saved_game.tres` to fail to load. Mark the Quest and Array[Quest] as not to be stored, and define stored proxy properties that convert to and from resource_path strings. This matches how these were previously stored. Similarly, the inventory is in terms of InventoryItem resources. Unlike Quests we don't store these in the game repo but define them inline wherever they are used. I think this is a bad design - it would be better to use just the ItemType enum throughout given that we hardcode exactly 3 items and 2 textures for each - but to avoid this PR getting even further out of hand, translate these to and from the ItemType enum member name when storing, as before. The result of jumping through these hoops is that the `saved_game.tres` only refers to the scripts that define aspects of the game state. I think that's acceptable coupling. I am disappointed to have to write quite a bit of what amounts to custom serialisation code - I was hoping to avoid that by using ResourceLoader/ResourceSaver - but it's still a net improvement I think. I did not make any attempt to migrate the old save game format to the new one. This game is small, the state has little impact, and this will also mean more people see the tutorial. :) Saving ------ Only save the state in a few specific places, rather than whenever parts of the state change (if we remembered to add a _save() call): - When a checkpoint is activated (so that if you quit the game with Alt+F4 after activating a checkpoint, it is kept); - When reloading the current scene, typically due to being defeated, to record the loss of life; - When switching to a new scene; - When you use the debug menu to tweak the completed quests. Resolves #2016

wjt mentioned this pull request May 26, 2026

“Change the main scene to your quest's first scene” doesn't work quite right with GameState #2250

Open

wjt linked an issue May 26, 2026 that may be closed by this pull request

Refactor game state #2016

Closed

wjt force-pushed the wjt/refactor-game-state branch from 6484f38 to ed15974 Compare May 26, 2026 16:47

wjt commented May 26, 2026

View reviewed changes

wjt force-pushed the wjt/refactor-game-state branch from ed15974 to 4004fdf Compare May 27, 2026 09:50

wjt changed the title ~~DRAFT: Refactor game state~~ [2/2] Refactor game state May 27, 2026

wjt force-pushed the wjt/refactor-game-state branch from 4004fdf to 782734e Compare May 27, 2026 16:32

wjt changed the base branch from main to wjt/storybook-handle-playing-no-edit-quest-tres-correctly May 27, 2026 16:32

wjt force-pushed the wjt/refactor-game-state branch 3 times, most recently from eb7e9fd to 7d2c84a Compare May 27, 2026 16:46

Base automatically changed from wjt/storybook-handle-playing-no-edit-quest-tres-correctly to main May 27, 2026 17:20

wjt changed the title ~~[2/2] Refactor game state~~ Refactor game state May 27, 2026

wjt marked this pull request as ready for review May 27, 2026 17:24

wjt requested review from a team as code owners May 27, 2026 17:24

manuq approved these changes May 27, 2026

View reviewed changes

wjt force-pushed the wjt/refactor-game-state branch from 7d2c84a to 0725700 Compare May 27, 2026 21:02

wjt force-pushed the wjt/refactor-game-state branch from 0725700 to ab7aeb9 Compare May 27, 2026 21:04

wjt merged commit 515115d into main May 27, 2026
6 checks passed

wjt deleted the wjt/refactor-game-state branch May 27, 2026 21:06

Conversation

wjt commented May 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

State API

Storage

Saving

Uh oh!

github-actions Bot commented May 21, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

manuq left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

wjt commented May 21, 2026 •

edited

Loading